✅ Deploy Preview for decent-interface-dev ready!

Name	Link
🔨 Latest commit	2d6f76d
🔍 Latest deploy log	https://app.netlify.com/sites/decent-interface-dev/deploys/676440cf89715d0008cd68bb
😎 Deploy Preview	https://deploy-preview-2644.app.dev.decentdao.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

✅ Deploy Preview for decent-interface-prod ready!

Name	Link
🔨 Latest commit	2d6f76d
🔍 Latest deploy log	https://app.netlify.com/sites/decent-interface-prod/deploys/676440cfa2fe22000876a9a9
😎 Deploy Preview	https://deploy-preview-2644.app.decentdao.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

I wanted to test each PR, but was unable to do so because the PRs do not contain enough context to be easily testable.

For each PR, included should be clear directions on how to test the PR prior to approval. No one should approve a PR without testing it.

For each PR, please add testing instructions so I can test them each and approve this prod push 🫡 I am on standby to do this any time.

@parkermccurley which PRs specifically do you need testing instructions for? Don't say "all of them".

@parkermccurley by 'test' each PR, you are saying you want to test what these PRs fix right? not that you are testing WITHIN those PRs (as in the PR's deploy preview)?

For testing. I guess the only one that isn't going to be clear is #2615, This is more or less a technical update and nothing to test for that PR

Yep @trainface I actually mean each one (I get some like the "My DAOs" color change are self-explanatory)
since we don't have unit test coverage and it isn't clear to me in each of these PRs what parts of the app they touch, how am I supposed to make sure the PR was implemented successfully? Including some basic instructions (a user story to run through) on PRs for reviewers should be default

@Da-Colon I'm referring to testing in the deploy previews where UI tests are necessary, yes

@Da-Colon For #2615 are there parts of the applications where the changes you have made are more or less likely to have been affected so I can do some QA and see if bugs were introduced

@parkermccurley All the individual changes are within this deploy-preview, I'd test them all here versus jumping around.

As far as changes in that PR, that contains all the stuff for network switching update.

Edit: I see the problem now. That PR is actually a collection of other PRs. I"ve added the links in that PR. I'll remember that (hopefully the last of its kinda) if not need to make sure for testing Purposes that roots get updated. But together those PRs make up the updated network switcher

@Da-Colon yep I'm also happy to test here on this deploy-preview, just think it will be easier (for developers) moving forward if PR authors add testing instructions in the PRs themselves instead of waiting until this point

Thanks for adding the links, your PRs generally have a good amount of context (screenshots for example) but even these PRs: #2638 #2641 for example I'm not sure where in the application I should be testing if these bugs have been removed. PRs should include that context for a reviewer to be able to drop in and test the functionality...

This is like the tip of the iceberg in terms of testing, and its still very possible we merge things to prod that have unwanted side effects and introduce bugs we will not catch, but at a minimum I think a reviewer needs to go to deploy-preview and see for themselves that the intended outcome is there, and I'm happy to be the person to do that when we go to prod but need more context to be an effective tester

@adamgall @Da-Colon maybe I just need to go back to basics here. How do you guys suggest I test this merge to prod, to make sure all of the changes you've made are functional on dev?

@parkermccurley

• [Issue #2623] Update base logo #2643 is a simple icon update that includes a screenshot
• Fix color of "My DAOs" button in header #2635 is a simple button color update which include links to slack messages that include screenshots
• ENG-45 | Fix Address xyz is invalid bug #2641 includes a link to the linear issue whose comments include the steps to reproduce the bug

it feels like a waste of developer time to include further testing instructions in those. i understand that in the future we should be more consistent with testing instructions, but for these PRs i'm going to say no, please click through the links to gain needed context.

I agree thanks @adamgall. For future PRs please make sure the team knows to include testing instructions (if it can be tested in the UI).

That leaves:

#2615 - @Da-Colon just checking again 'cause its a big ol' PR, are there any parts of the application that may have been made fragile by these updates that are worth kicking the tires on?

#2639 - @Da-Colon I'm just not sure how to test it

#2642 - @Da-Colon also not sure how to test it

cc @johnqh

updated the PR description of #2615 to include the test cases. @parkermccurley

"For future PRs please make sure the team knows to include testing instructions"

@parkermccurley if by this you mean the instructions should be in the PR description, I'm gonna have to push back. That's what the linked Linear issue is there for -- ALL the context one would need, which ideally would cover even more than just the one small "happy" path to reproduce the bug esp if there's been discussion in there.

If the linked issue doesn't have enough to reliably reproduce the issue then the failure is on the issue description, not the PR description. I see the PR description as dev facing with comments that code reviewers would/should be interested in. In the same vein, if a dev wants to UI/UX test a PR, then they should also go look at the Linear issue.

If there's no linked issue . . . well. There should have been. I think we'd better solidify the habit of creating issues for EVERYTHING we push a fix/update for, even if it seems trivial. cc @decentdao/engineering

Thanks @Da-Colon! Approving this.

If the linked issue doesn't have enough to reliably reproduce the issue then the failure is on the issue description, not the PR description. I see the PR description as dev facing with comments that code reviewers would/should be interested in. In the same vein, if a dev wants to UI/UX test a PR, then they should also go look at the Linear issue.

If there's no linked issue . . . well. There should have been. I think we'd better solidify the habit of creating issues for EVERYTHING we push a fix/update for, even if it seems trivial. cc @decentdao/engineering

@DarksightKellar I hear you, this makes sense to me. On teams I've worked on in the past, testing instructions were included in Jira issues for PRs in BitBucket, so that is aligned with my experience.

I'm not going to be reviewing code and offering useful input there. For me it is important that before we push to prod I'm confident what we are launching (and what I will be promoting to our users) meets our standards via hands on UI testing. I find this especially important in the absence of test coverage in the code base.

So if I wanted to do this for this push, I'd need to:

• Go into every PR
• Gather context for what the PR does (remember, I'm not contributing to the codebase - I lack so much context therefore what may seem simple and clear to someone working on this codebase daily, is not simple or clear to me)
• Click into the Linear issue
• Understand how to test the issue
• Some cycles of back-and-forth with devs to understand what I should be testing
• Go to deploy preview and test it

This makes it a 0.5-1 day process, even if the actual work of testing is only 10 minutes.

What I'd really like to see on a prod push is:

• Plain english: X was changed in our codebase, and Y should be the result

So I can just get on app.dev and mess around with it before we push to prod. This should take 30-60 minutes per prod push.

What's the best way to make that UX for me as a reviewer? Would it be creating a linear ticket that includes all that information for each prod push?